Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes issue https://github.com/FreezingMoon/AncientBeast/issues/2494 #2508

Merged
merged 2 commits into from
Oct 18, 2023

Conversation

tomaszajac-vs
Copy link

This fixes issue #2494

@vercel
Copy link

vercel bot commented Oct 14, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
ancientbeast ✅ Ready (Inspect) Visit Preview Oct 18, 2023 8:16am

@ghost
Copy link

ghost commented Oct 14, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@allmtz
Copy link
Contributor

allmtz commented Oct 15, 2023

@tomaszajac-vs Great work!

A couple of things to consider:

I think it would be less error prone to have flipped only exist on player. Duplicating it on player and creature introduces the possibility of these two getting out of sync.

The logic inside getHexMap and getHexLine will most likely need to be refactored. I haven't tested the current changes but last time I tried passing the correct value of flipped to getHexMap and getHexLine, a lot of the targeting functionality broke.

@tomaszajac-vs
Copy link
Author

Do you propose to only use the flipped property from the player for all of the creatures?

@DreadKnight
Copy link
Member

I need to keep in mind what @allmtz said, duplicated stuff that can get out of sync is a bad thing indeed. When I'll test this soon will have to pay attention to the unit abilities.

@allmtz
Copy link
Contributor

allmtz commented Oct 16, 2023

Do you propose to only use the flipped property from the player for all of the creatures?

I think this makes sense. A player is either flipped (i.e. on the right side of the screen) or not (on the left side) and any creatures the player summons will inherit that flipped status.

Each creature already has a player property so acessing flipped would look like creature.player.flipped.

The benefit I can think of in keeping a copy of the flipped value on a creature is for something like a status effect that only effects that creature. Even so, the same could be accomplished by creating something like a isDisoriented flag on creature. Other than this, I don't see a reason to have the flipped status exists on a creature independent from the player that summoned it.

@DreadKnight
Copy link
Member

Will mark this as draft until @allmtz's suggestion gets implemented, as it makes a lot of sense to me.

@DreadKnight DreadKnight marked this pull request as draft October 16, 2023 17:43
@tomaszajac-vs
Copy link
Author

Hi, I have changed the reference to the player.flipped on all of the creatures

@DreadKnight
Copy link
Member

@tomaszajac-vs Seems to work fine as far as I tested so far 👍🏻

@DreadKnight DreadKnight merged commit ea7fc27 into FreezingMoon:master Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

creature.flipped is undefined
3 participants